Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.80%. Comparing base (c45b9eb) to head (8da633b).

@@           Coverage Diff           @@
##             main    #7732   +/-   ##
=======================================
  Coverage   59.80%   59.80%           
=======================================
  Files         352      352           
  Lines       31805    31806    +1     
=======================================
+ Hits        19022    19023    +1     
  Misses      12783    12783           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@microsoft-github-policy-service agree

Hi Andreas,

Thank you for this contribution. I had also discovered this lack of capability and had been thinking about ways to approach the problem.

After looking at your code, I have a few thoughts:

• The first note I have is that it seems like this code changes the expected shape of the output dataset depending on whether or not the setpoints has a has_control_of relationship or not. I consider this unwanted behavior, as ideally the dataset shape should not care if there are additional, parallel axes of data being saved.
• Second, I note that this new code does not handle more than a single layer of has_control_of indirection. What is here is still better than no levels of indirection, but one of the use-cases I have been pondering could have multiple, nested levesl.
• Finally, I am having a little trouble following the broadcasting logic in the new unpack_self call. It seems like each has_control_of parameter gets broadcast against the other original setpoints. It's not obvious to me that this is an intuitive way to store the data. On the other hand, the most intuitive (and performant) way to store the mappings is to not broadcast at all and simply store the individual 1D mappings (or as low-dimensional reduction as possible) directly. For readability, I'm also thinking it may be worth splitting the broadcasting/has_control_of logic into its own private method (with suitable docstring).

This last point is pretty sticky, and it's where I've struggled the most. In a high-dimensional space, or one in which one axis is significantly larger than the others (eg. where one axis is time), we could end up creating a large number of matrices to save to the dataset that are highly redundant.

Curious to hear your thoughts on these issues.

Thanks again!

• Samantha

Hi, Samantha,

Thanks a lot for your feedback. I am very happy to hear that you found the same issue.

Some thoughts on the points you mentioned:

• The shape of the dataset (as in measured parameters) does not change with this implementation. It just adds the dependent parameters in the unpacking in the exact same fashion as the direct setpoints are stored. I agree that this adds quite some redundancy but I didnt want to change how the ParameterWithSetpoints stores its data.
• I agree that all inferred parameters should be included. I am just not sure how to implement that best since the shaping of the parameter data depends on the shape of the setpoints of the ParameterWithSetpoints.
• The third point is partially covered by the first question. A ParameterWithSetpoints always requires a full meshgrid for each parameter in order to construct coordinates explicitly for every data point (pixel). I think overthrowing this will be quite a significant change. I agree with splitting the inferred parameter unpacking to a separate method! I will change that as soon as I find time.

Thanks again for your feedback. Curious what you are thinking!

Cheers,
Andreas